Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Issue #2732] clear search input when clicking search nav link #2756

Merged
merged 5 commits into from
Nov 14, 2024

Conversation

doug-s-nava
Copy link
Collaborator

@doug-s-nava doug-s-nava commented Nov 6, 2024

Summary

Fixes #2732

Time to review: 10 mins

Changes proposed

Fixes a bug where the search bar does not clear when clicking "search" on the nav while on the search page.

The solution I came up with was to append a query param to the search nav item when on the search page, whose presence is a signal to the page to clear the search bar.

The approach is not ideal, but a fix requires knowing when a navigation has occurred, and Next does not provide a simple way to know that without relying on something like global state.

Context for reviewers

Test steps

  1. on this branch, visit the search page
  2. enter text in the search bar
  3. click the "search" nav item
  4. VERIFY the search bar is cleared when the page refreshes

Additional information

Screenshots, GIF demos, code examples or output to help show the changes working as expected.

@doug-s-nava doug-s-nava marked this pull request as ready for review November 8, 2024 21:16
Copy link
Contributor

@emilycnava emilycnava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed it works as expected, left a few thoughts. If this is the approach we wanna take, feel free to merge. I do think this bug may raise bigger questions about state management on the search page since I know there's discussions around how to handle having 'forcasted' and 'posted' checked by default too, so I think we just have some complex state on this particular page and may be worth doing a wholistic eval?

@@ -6,6 +6,7 @@
"node": ">=20.0.0"
},
"scripts": {
"all-checks": "npm run test && npm run lint && npm run ts:check && npm run build",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love this!! kudos!

@@ -36,6 +52,7 @@ export default function SearchBar({ query }: SearchBarProps) {
</label>
<div className="usa-search usa-search--big" role="search">
<input
ref={inputRef}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non blocking: Is it worth just adding either useState or possibly the newer server action state management for this form as opposed to using refs and essentially handling setting state manually?

Copy link
Collaborator

@mdragon mdragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, per Emily's review

@doug-s-nava
Copy link
Collaborator Author

Note that @emilycnava and I discussed offline, both agree that this solution doesn't feel great but that we don't have a better idea without creating some global state or other relatively heavy solution that would be out of scope for this ticket. We should review this with whoever is interested and figure out what to do with this sort of thing as a next step.

@doug-s-nava doug-s-nava merged commit 081325f into main Nov 14, 2024
10 of 11 checks passed
@doug-s-nava doug-s-nava deleted the dschrashun/2732-clear-search-bar branch November 14, 2024 17:50
@andycochran
Copy link
Collaborator

Could we use router.reload() or something to force a page refresh when you click the nav menu?

@doug-s-nava
Copy link
Collaborator Author

@andycochran I'll look into that, if we're ok with doing a full page refresh in this case

return [
{ text: t("nav_link_home"), href: "/" },
getSearchLink(path.includes("/search")),
{ text: t("nav_link_process"), href: "/process" },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't get a chance to review this, but this could be simplified by just including an array of links instead of an array of objects that is then mapped to links. Something for future clean-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clear search bar text with a new search (e.g. clicking "Search" on nav)
5 participants